Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Lazy cache for originalhost #1766

Merged
merged 3 commits into from
Apr 30, 2024
Merged

Add Lazy cache for originalhost #1766

merged 3 commits into from
Apr 30, 2024

Conversation

karim-z
Copy link
Contributor

@karim-z karim-z commented Apr 19, 2024

Add Lazy cache for originalhost when parsing headers

@@ -510,12 +514,23 @@ protected String generateInfoForLogging() {
@Override
public String getOriginalHost() {
try {
return getOriginalHost(getHeaders(), getServerName());
if (!CACHE_ORIGINAL_HOST.get()) {
return generateOriginalHost(getHeaders(), getServerName());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we need this FP to not-cache the original host; since it's intended for fetching the host on the inbound request, it shouldn't be mutated by any header processing, so keeping it always cached makes it simpler

} catch (URISyntaxException e) {
throw new IllegalArgumentException(e);
}
}

String generateOriginalHost(Headers headers, String serverName) throws URISyntaxException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think you can remove this method and just call getOriginalHost(headers, serverName) from within the getOriginalHost() method

@@ -510,7 +511,10 @@ protected String generateInfoForLogging() {
@Override
public String getOriginalHost() {
try {
return getOriginalHost(getHeaders(), getServerName());
if (originalHost == null) {
originalHost = getOriginalHost(getHeaders(), getServerName());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compute intensive part here should be the parseHostHeader logic.
Consider the option to just cache that part and not the X-Forwarded-Host part.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

X-Forwarded-Host part is called everytime before the parseHostHeader and it loops on all existing headers to check for the x-forwarded-host using headers.getFirst before attempting to call the parseHostHeader. Doesn't sound efficient. I am tempted to cache that as well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, it does iterate through the list, ugh.

@karim-z karim-z merged commit 28d7f60 into master Apr 30, 2024
5 checks passed
argha-c pushed a commit that referenced this pull request Sep 10, 2024
* Lazy cache originalhost

* Update toString function

* Update unittests

---------

Co-authored-by: $(git --no-pager log --format=format:'%an' -n 1) <$(git --no-pager log --format=format:'%ae' -n 1)>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants